-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(recruit): 내 공고 생성 모달 구현 #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굿 짧은 코멘트 남겼습니다~
오타난 것만 고치면 될 것 같아 approve 합니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shadn
=> Shadcn
오타난 것 같아용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f77f66f 여기서 수정했어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 무슨 훅인가요?? 설명 부탁드립니다 ^_^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 만들다가 만 코드입니다! 이후에 샤든 ui걷어내고 로직 교체하려구요!
function Dialog({ open, children, onClose }: DialogProps) { | ||
return ( | ||
<RadixDialog.Root> | ||
<RadixDialog.Portal> | ||
<RadixDialog.Portal> | ||
<RadixDialog.Overlay></RadixDialog.Overlay> | ||
<RadixDialog.Content>{children}</RadixDialog.Content> | ||
</RadixDialog.Portal> | ||
</RadixDialog.Portal> | ||
</RadixDialog.Root> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 파일도 정체가 궁금합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위와 동일합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니당
코멘트 남겨드렸어요~~!
<span className={clsx('text-label1', period === selectedPeriod ? 'text-neutral-30' : 'text-neutral-80')}> | ||
{period} | ||
</span> | ||
{period === selectedPeriod ? <Icon size={16} name="check" color="#AEB0B6" /> : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If 컴포넌트 어떠신가용?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
56116da 여기서 반영했어요
src/app/(sidebar)/my-recruit/components/NewRecruitDialogContent/NewRecruitDialogContent.tsx
Show resolved
Hide resolved
{required ? <div className="text-mint-40 text-label1">*</div> : null} | ||
<input value={value} className="flex-1 outline-none bg-transparent" {...inputProps} /> | ||
{right != null ? <div className="ml-[12px]">{right}</div> : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위 If문과 동일해요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
56116da 여기서 반영했어요
return ( | ||
<div className="w-full flex justify-between items-center p-12 bg-neutral-1 border-neutral-20 rounded-[8px] border-[1px]"> | ||
{required ? <div className="text-mint-40 text-label1">*</div> : null} | ||
<input value={value} className="flex-1 outline-none bg-transparent" {...inputProps} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공통 Input 컴포넌트를 사용하시지 않은 이유가 있나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컴포넌트명은 TextFiled인데 태그가 사용되고 있어서 혼란이 있을 수도 있다고 생각해요
textarea를 사용하거나 InputField라는 네이밍은 어떨까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공통 Input 컴포넌트를 사용하시지 않은 이유가 있나요??
이 PR에는 input컴포넌트가 아직 없어요! 이후 교체하겠습니다!
textarea를 사용하거나 InputField라는 네이밍은 어떨까요??
회사에서 습관적으로 쓰다보니 습관적으로 쓰게 되었네요. 좋습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
56116da 여기서 반영했어요
|
||
export default function MyRecruit() { | ||
const [dialogOpened, setDialogOpened] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용되지 않는 변수로 보입니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a61b3a5 여기서 제거했습니다
fill-rule="evenodd" | ||
clip-rule="evenodd" | ||
d="M10.705 1.6194V0.273682H9.53137V3.83357H8.74898V1.6194H4.86048V0.273682H3.68689V3.83357H2.9045V1.6194H0.0957031V5.63308H13.9049V1.6194H10.705Z" | ||
fill={color} | ||
/> | ||
<path | ||
fill-rule="evenodd" | ||
clip-rule="evenodd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 카멜 케이스로 해주시면 좋을 것 같아요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 놓친부분 감사합니다!
f46c9ee 여기서 반영했어요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
꾸벅 (--)(__) 수고하셨습니다 다른 리뷰 답변 달리면 다시 보러 오겠습니다
<div className="w-full flex justify-between items-center p-12 bg-neutral-1 border-neutral-20 rounded-[8px] border-[1px]"> | ||
{required ? <div className="text-mint-40 text-label1">*</div> : null} | ||
<input value={value} className="flex-1 outline-none bg-transparent" {...inputProps} /> | ||
{right != null ? <div className="ml-[12px]">{right}</div> : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
잘 몰라서 여쭙는데, 보통 텍스트 필드 오른쪽에 표시되는 글자수 체킹 혹은 아이콘을 right라는 이름으로 짓는 것이 일반적인가요??
사용된 컴포넌트를 확인하고서 이 props의 역햘을 이해할 수 있었어서요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
들어가는 컴포넌트들이 통일되지않아 특정한 이름을 짓는 것이 어려워서 제너럴한 이름으로 가져가게 되었습니다!
리뷰주신 부분들 모두 반영해서 어프룹해보겠습니다! |
* feat(recruit): 내 공고 생성 모달 구현 * update
* feat * feat * 중복 처리 * fix * f * feat * feat(recruit): 내 공고 생성 모달 구현 (#20) * feat(recruit): 내 공고 생성 모달 구현 * update * feat(editor): 에디터의 제목, 태그 구현 (#19) * feat * feat * 중복 처리 * fix * f * fix * import * fix * a * fix ui * fix * fix * fix * fix * fix * fix * fix * yarn * fix * fix * feat * feat * feat * feat * feat * fix * fix * f * fix * fix * f --------- Co-authored-by: shellboy <[email protected]>
* feat * feat * 중복 처리 * fix * f * feat * feat(recruit): 내 공고 생성 모달 구현 (#20) * feat(recruit): 내 공고 생성 모달 구현 * update * feat(editor): 에디터의 제목, 태그 구현 (#19) * feat * feat * 중복 처리 * fix * f * fix * import * fix * a * fix ui * fix * fix * fix * fix * fix * fix * fix * yarn * fix * fix * feat * feat * feat * feat * feat * fix * fix * f * fix * fix * f --------- Co-authored-by: shellboy <[email protected]>
3. 관련 스크린샷을 첨부해주세요.
2024-08-04.4.34.28.mov
4. 완료 사항
5. 추가 사항 / 코드 리뷰 받고 싶은 부분